Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GPU] Add chained reshape support for scf.forall expand destination pattern #19597

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

nirvedhmeshram
Copy link
Contributor

@nirvedhmeshram nirvedhmeshram commented Jan 3, 2025

Currently when expanding scf.forall we make trivially foldable expand.shape ops (same source and destination ranks) with a wrong reassociation map. However, if there is another expand.shape consumer to these ops then the upstream ComposeReassociativeReshapeOps can merge these into a wrong expand.shape op that leads to error. So we just replace the uses of these expand op to avoid this issue.

@nirvedhmeshram
Copy link
Contributor Author

nirvedhmeshram commented Jan 3, 2025

This is the error that was discovered that this PR fixes
https://github.com/iree-org/iree/actions/runs/12590168915/job/35091417254?pr=19520#step:7:1102

Copy link
Contributor

@Max191 Max191 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to replace uses of these trivial reshapes instead of generating the correct reshapes to begin with? Where are we creating these reshapes with incorrect reassociation maps?

@nirvedhmeshram
Copy link
Contributor Author

nirvedhmeshram commented Jan 7, 2025

Is there a reason to replace uses of these trivial reshapes instead of generating the correct reshapes to begin with? Where are we creating these reshapes with incorrect reassociation maps?

We dont create them, we replace the producer of the reshapes to have the same result type as the reshapes (after doing proper verification) making them trivial, but with wrong (old) reassociations...

@nirvedhmeshram nirvedhmeshram requested a review from Max191 January 7, 2025 20:03
Copy link
Contributor

@Max191 Max191 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for jogging my memory. This makes sense to me now!

@nirvedhmeshram nirvedhmeshram merged commit 9a83239 into iree-org:main Jan 7, 2025
40 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants